[scout] extend login functionality with Kibana built-in roles#271992
Draft
dmlemeshko wants to merge 6 commits into
Draft
[scout] extend login functionality with Kibana built-in roles#271992dmlemeshko wants to merge 6 commits into
dmlemeshko wants to merge 6 commits into
Conversation
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
dmlemeshko
added
release_note:skip
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review notes on the Scout built-in role support:
getApiKeyForBuiltinRoleprovisions acustom_role_worker_NES role that the API key never uses (the descriptor is embedded inline). For API-only consumers this is an extraPUT /_security/roleper call plus cleanup overhead.samlAuth.setBuiltinRoleonly stripsmetadata/transient_metadata/description; ES roles can carry other fields (notablyglobal) that the role API accepts but the API keyrole_descriptorsendpoint rejects. An allow-list of supported descriptor fields would be more forward-compatible.- New
browserAuth.loginWithBuiltinRoleis not exercised by the newbuilt_in_roles.spec.ts; the API/SAML methods are covered but the UI fixture path is not.
Non-blocking — leaving comments inline for context.
Generated by Claude Reviewer for issue #271992 · ● 12.1M
Comment on lines
+219
to
+224
| const getApiKeyForBuiltinRole = async (roleName: string): Promise<RoleApiCredentials> => { | ||
| const descriptor = await samlAuth.setBuiltinRole(roleName); | ||
| return createApiKeyWithAdminCredentials(samlAuth.customRoleName, { | ||
| [samlAuth.customRoleName]: descriptor, | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
setBuiltinRole calls setCustomRole internally, which performs a PUT /_security/role/custom_role_worker_N to provision the role in ES (and a corresponding DELETE at worker cleanup). However, this ES role is never used here — the API key embeds the role_descriptors inline. For API-only consumers, this is an unnecessary round trip per call (and per worker, an unnecessary cleanup).
Consider exposing a lower-level helper that just fetches and strips the descriptor (without the setCustomRole side effect), and only provision the role lazily when login/SAML cookies are actually needed. The current behavior is correct, just wasteful when only API keys are required.
| expect(apiKey.name).toBeDefined(); | ||
| expect(apiKeyHeader.Authorization).toMatch(/^ApiKey /); | ||
| } | ||
| ); |
Contributor
There was a problem hiding this comment.
This file covers samlAuth.setBuiltinRole and requestAuth.getApiKeyForBuiltinRole, but there is no test for browserAuth.loginWithBuiltinRole, which is the third new public method introduced by this PR. Since it has its own user-facing contract (returns a cookie via loginAs(samlAuth.customRoleName) after setBuiltinRole), a small UI test would close the coverage gap and protect the browser-auth path against regressions.
Comment on lines
+88
to
+93
| const { | ||
| metadata: _metadata, | ||
| transient_metadata: _transient, | ||
| description: _description, | ||
| ...descriptor | ||
| } = roleData; |
Contributor
There was a problem hiding this comment.
The stripping list only removes metadata, transient_metadata, and description. ES getRole can return additional fields (e.g. global, remote_cluster, remote_indices) depending on the role. global in particular is accepted by the role-management API but is not accepted by the API key role_descriptors endpoint, so for any built-in role that defines a global block (or future roles that add one), getApiKeyForBuiltinRole will fail with an opaque 400.
Consider either (a) allow-listing the known-supported descriptor fields (cluster, indices, applications, run_as, remote_cluster, remote_indices) instead of deny-listing, or (b) documenting the limitation so callers know which roles are safe to pass through. Option (a) makes the surface forward-compatible with new ES role fields.
Contributor
⏳ Build in-progress, with failures
Failed CI Steps
Test Failures
History
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Many FTR tests rely on Kibana build-in roles and it makes sense to add its support to Scout. It should simplify
FTR -> Scoutmigration:UI tests auth flow:
API tests auth flow:
Note: since
build-inroles are available only in stateful, the error will be thrown when you call it in tests running serverless env